Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend MembersThat and MembersShould to have starting,… (Resolves #239) #314

Merged
merged 2 commits into from
May 24, 2020
Merged

Conversation

kamer
Copy link

@kamer kamer commented Feb 11, 2020

… containing and ending functionality.

It's my first attempt to contribute an open-source project.
Hope this solves the issue properly.

Resolves: #239

@ghost
Copy link

ghost commented Feb 11, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.193 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

@hankem
Copy link
Member

hankem commented Feb 11, 2020

Cool, thanks! 😉
As I was curious, I had a first quick look and two tiny comments.

@@ -107,6 +107,33 @@
@PublicAPI(usage = ACCESS)
CONJUNCTION haveFullNameNotMatching(String regex);

/**
* Asserts that members have a full name starting with given prefix.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, the new methods test member names, but not full names (which would include their owners' FQCN).

$(described(methods().that().haveNameEndingWith("C")), ImmutableSet.of(METHOD_C)),
$(described(constructors().that().haveNameEndingWith("it>")), ALL_CONSTRUCTOR_DESCRIPTIONS),
$(described(fields().that().haveNameEndingWith("B")), ImmutableSet.of(FIELD_B)),

Copy link
Member

@hankem hankem Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably add similar test coverage for the members should functionality in MembersShouldTest.

@kamer
Copy link
Author

kamer commented Feb 12, 2020

Update:

  • Comments in MembersShould is changed since new methods test name instead of fullName.
  • Tests added in MembersShouldTest.

@codecholeric codecholeric self-requested a review April 9, 2020 14:29
@codecholeric codecholeric self-assigned this Apr 9, 2020
Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution, I really appreciate it! 😃 Sorry that it took me so long to look into it, my pipeline was unfortunately full 😞
I added two minor review commits and one bigger commit where I added the negated forms. I've noticed I forgot to add those to the issue, but to be consistent I think it makes sense.
Can you look at my commits and tell me if that looks okay to you?
If yes I would suggest you squash your commits and my little review commits into one commit (containing the positive additions to the syntax) and then keep my last commit (adding the negated version) as a separate commit on top. That way we have semantically relevant commits for the master commit history 😉

Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: Can you double check your signed-off part? It seems a little weird, your signature is always

Signed-off-by: kamer <kamer@kamerelciyar.com>

But your author name in the commits changes from Kamer Elciyar to kamer which the DCO check doesn't like I guess 😉 But when you squash your commits it might be easy to fix.
If this is all not completely clear to you, simply ask! 😃

@codecholeric
Copy link
Collaborator

One thing I noticed though (out of scope for this PR) is that we really should look into this zoo of ArchConditions that really only differ in this little this of how to create the positive or negative description of a failure. Sometimes we have satisfied ? predicate.getDescription().replace(..) : ... sometimes it's hard-coded in the description and duplicated, just seems to be a lot of different ways and duplication just to handle this details creation. Maybe DescribedPredicate should be extended a little so the description is more sophisticated and can handle different grammatical configurations...

@codecholeric
Copy link
Collaborator

@kamer Any update?

@kamer
Copy link
Author

kamer commented May 24, 2020

@codecholeric I updated like you have described and build with ./gradlew clean build -PallTests as it is recommended in contributing guide. I built successfully locally but build checks fail. Am I doing something wrong?

Kamer Elciyar and others added 2 commits May 24, 2020 14:12
… ending functionality. (#239)

Signed-off-by: Kamer Elciyar <kamer@kamerelciyar.com>
I have also added some tests for the failure details, since broken messages there would be undetected at the moment. The reason that some of these tests for members are missing for other methods is simply that we reuse those for classes, so they were not added in the past since those tests would have already caught it. If we have syntax methods explicitly for members (that are not also used for classes), then we should add a detailed test checking that the failure details indeed match.
when I wrote those tests I also noticed that messages like `Constructor<very.long.fqn.<init>()> does not start with 'foo'` does also read slightly weird (because there is no focus on the "name" the "starts with" refers to) so I added "name" to the failure details, i.e. `Constructor<..> name does not start with...`.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
@codecholeric
Copy link
Collaborator

@kamer thanks for cleaning it up!! 😃 When I tested the branch locally the tests also didn't pass, because of changed logic with respect to the line number -> #344
I've fixed it real quick. (I touched those tests anyway in my last commit, so it was a simple one line fixup in the assertion)

I adjusted the commit message of your commit and removed all the blubber from my review commits, because I do not think these comments will provide much benefit (adjust javadoc, etc.), when I read this history entry in 6 months 😉 (you can adjust the message, when you squash commits, you don't have to append them all to one long sequence)
Also I noticed that you did a merge master -> feature-branch, I normally always try to do a rebase + force push (i.e. git pull --rebase upstream master + git push --force-with-lease). The consequence is, that the history will stay linear when you merge back into master, which I highly appreciate. I've adjusted this on your branch, so instead of the merge commit, the branch is now rebased on upstream master

Anyway, thank you so much for your support!! Gonna merge it now 😃

@codecholeric codecholeric self-requested a review May 24, 2020 12:21
@codecholeric codecholeric merged commit 93b3767 into TNG:master May 24, 2020
codecholeric added a commit that referenced this pull request Feb 21, 2021
#314

… containing and ending functionality and their negations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consistent methods for classes() and methods()
3 participants